SearchableSnapshotsIntegTests should wait for shard index folder to be cleaned up #80341
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tlrx
added
>test
elasticmachine
added
the
Team:Distributed (Obsolete)
|
Pinging @elastic/es-distributed (Team:Distributed) |
tlrx
commented
Nov 4, 2021
| @@ -75,7 +77,7 @@ protected boolean addMockInternalEngine() { | |||
|
|
|||
| @Override | |||
| protected Collection<Class<? extends Plugin>> nodePlugins() { | |||
| return List.of(LocalStateSearchableSnapshots.class); | |||
| return CollectionUtils.appendToCopy(super.nodePlugins(), LocalStateSearchableSnapshots.class); | |||
There was a problem hiding this comment.
This is independant but looks wrong so I fixed it.
arteam
approved these changes
Nov 4, 2021
|
Thanks Artem! |
tlrx
added a commit
to tlrx/elasticsearch
that referenced
this pull request
Nov 5, 2021
The tests testCreateAndRestoreSearchableSnapshot and testCreateAndRestorePartialSearchableSnapshot both failed once when asserting the shard folders using assertShardFolders(index, true). The failures occurred when the original index is first closed (not deleted) and mounted again under the same name (so it will be restored as a searchable snapshot index on top of the existing shard files). The SearchableSnapshotDirectory implementation takes care to clean up the shard files on disk using SearchableSnapshotDirectory.cleanExistingRegularShardFiles() and the tests verify that the shard index folder is indeed deleted from disk on all nodes but sometime fail because the folder is still present. I wasn't able to reproduce but I think that the closing of the original index + the creation of the .snapshot-blob-cache index trigger some shard relocations that are cancelled by the subsequent mount/restore, leaving some files on disk that should be cleaned up but maybe not immediately. This commit changes the tests to assertBusy() when verifying the shard folders and also adds more logging information in case waiting for the assertShardFolders(index, true) is not enough. Closes elastic#77831
💔 Backport failed
You can use sqren/backport to manually backport by running |
elasticsearchmachine
pushed a commit
that referenced
this pull request
Nov 5, 2021
The tests testCreateAndRestoreSearchableSnapshot and testCreateAndRestorePartialSearchableSnapshot both failed once when asserting the shard folders using assertShardFolders(index, true). The failures occurred when the original index is first closed (not deleted) and mounted again under the same name (so it will be restored as a searchable snapshot index on top of the existing shard files). The SearchableSnapshotDirectory implementation takes care to clean up the shard files on disk using SearchableSnapshotDirectory.cleanExistingRegularShardFiles() and the tests verify that the shard index folder is indeed deleted from disk on all nodes but sometime fail because the folder is still present. I wasn't able to reproduce but I think that the closing of the original index + the creation of the .snapshot-blob-cache index trigger some shard relocations that are cancelled by the subsequent mount/restore, leaving some files on disk that should be cleaned up but maybe not immediately. This commit changes the tests to assertBusy() when verifying the shard folders and also adds more logging information in case waiting for the assertShardFolders(index, true) is not enough. Closes #77831
tlrx
added a commit
to tlrx/elasticsearch
that referenced
this pull request
Nov 5, 2021
The tests testCreateAndRestoreSearchableSnapshot and testCreateAndRestorePartialSearchableSnapshot both failed once when asserting the shard folders using assertShardFolders(index, true). The failures occurred when the original index is first closed (not deleted) and mounted again under the same name (so it will be restored as a searchable snapshot index on top of the existing shard files). The SearchableSnapshotDirectory implementation takes care to clean up the shard files on disk using SearchableSnapshotDirectory.cleanExistingRegularShardFiles() and the tests verify that the shard index folder is indeed deleted from disk on all nodes but sometime fail because the folder is still present. I wasn't able to reproduce but I think that the closing of the original index + the creation of the .snapshot-blob-cache index trigger some shard relocations that are cancelled by the subsequent mount/restore, leaving some files on disk that should be cleaned up but maybe not immediately. This commit changes the tests to assertBusy() when verifying the shard folders and also adds more logging information in case waiting for the assertShardFolders(index, true) is not enough. Closes elastic#77831
elasticsearchmachine
pushed a commit
that referenced
this pull request
Nov 8, 2021
The tests testCreateAndRestoreSearchableSnapshot and testCreateAndRestorePartialSearchableSnapshot both failed once when asserting the shard folders using assertShardFolders(index, true). The failures occurred when the original index is first closed (not deleted) and mounted again under the same name (so it will be restored as a searchable snapshot index on top of the existing shard files). The SearchableSnapshotDirectory implementation takes care to clean up the shard files on disk using SearchableSnapshotDirectory.cleanExistingRegularShardFiles() and the tests verify that the shard index folder is indeed deleted from disk on all nodes but sometime fail because the folder is still present. I wasn't able to reproduce but I think that the closing of the original index + the creation of the .snapshot-blob-cache index trigger some shard relocations that are cancelled by the subsequent mount/restore, leaving some files on disk that should be cleaned up but maybe not immediately. This commit changes the tests to assertBusy() when verifying the shard folders and also adds more logging information in case waiting for the assertShardFolders(index, true) is not enough. Closes #77831
arteam
added a commit
to arteam/elasticsearch
that referenced
this pull request
Mar 14, 2022
Relates elastic#80341 Closes 84158
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The tests
SearchableSnapshotsIntegTests.testCreateAndRestoreSearchableSnapshotandFrozenSearchableSnapshotsIntegTests.testCreateAndRestorePartialSearchableSnapshotboth failed once when asserting the shard folders usingassertShardFolders(index, true).The failures occurred when the original index is first closed (not deleted) and mounted again under the same name (so it will be restored as a searchable snapshot index on top of the existing shard files). The
SearchableSnapshotDirectoryimplementation takes care to clean up the shard files on disk usingSearchableSnapshotDirectory.cleanExistingRegularShardFiles()and the tests verify that the shard index folder is indeed deleted from disk on all nodes but sometime fail because the folder is still present.I wasn't able to reproduce but I think that the closing of the original index + the creation of the
.snapshot-blob-cacheindex trigger some shard relocations that are cancelled by the subsequent mount/restore, leaving some files on disk that should be cleaned up but maybe not immediately.This pull request changes the tests to
assertBusy()when verifying the shard folders and also adds more logging information in case waiting for theassertShardFolders(index, true)is not enough.Closes #77831